feat(auth-service): nonce-based CSP and 5 security cucumber scenarios#100
feat(auth-service): nonce-based CSP and 5 security cucumber scenarios#100
Conversation
🦋 Changeset detectedLatest commit: 56e73fd The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes introduce per-request CSP nonce generation for inline scripts in the auth-service and implement deny-by-default metrics endpoint authentication. Middleware generates and stores nonces for page templates to include in script tags, while a new metrics auth helper validates HTTP Basic credentials. Corresponding security tests and feature scenarios verify nonce inclusion and metrics endpoint protection. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthService as Auth Service
participant Middleware as Security Headers<br/>Middleware
participant RouteHandler as Route Handler
participant Template as Template<br/>Renderer
participant Browser
Client->>AuthService: GET /auth/login
AuthService->>Middleware: Process request
Middleware->>Middleware: Generate nonce<br/>(base64url)
Middleware->>AuthService: Store in res.locals.cspNonce
AuthService->>RouteHandler: Route to handler
RouteHandler->>Template: Pass cspNonce from res.locals
Template->>Template: Render HTML with<br/>nonce in script tag
RouteHandler->>AuthService: Return rendered HTML
AuthService->>AuthService: Set CSP header with<br/>nonce-{value} in script-src
AuthService->>Browser: HTTP response + CSP header
Browser->>Browser: Parse CSP: script-src<br/>includes nonce-{value}
Browser->>Browser: Execute inline script only<br/>if nonce matches
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR spans multiple cohorts with new non-trivial logic (nonce generation, timing-safe auth comparison), introduces new helper modules with test coverage, and modifies route handlers and templates across several files. However, changes follow consistent patterns (nonce threading through middleware → routes → templates) and are largely additive without complex refactoring. Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ePDS-pr-100 environment in ePDS
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/auth-service/src/routes/choose-handle.ts (1)
315-326:⚠️ Potential issue | 🟠 MajorInclude the nonce in this error render path.
This catch branch is the only POST error render that omits
res.locals.cspNonce, so the returned handle picker page will have a nonce-only CSP but a non-nonced inline script.🔒 Proposed fix
renderChooseHandlePage( handleDomain, 'Could not verify handle availability. Please try again.', res.locals.csrfToken, showRandomButton, customCss, + res.locals.cspNonce as string, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 315 - 326, The error branch that returns the handle picker omits the CSP nonce, causing a mismatch between the page CSP and inline scripts; update the render call in the catch block so renderChooseHandlePage receives res.locals.cspNonce (like other error paths do) — locate the catch that logs with logger.error({ err, fullHandle }, 'Failed to check handle availability') and pass res.locals.cspNonce into the renderChooseHandlePage invocation along with handleDomain, the error message, res.locals.csrfToken, showRandomButton, and customCss.packages/auth-service/src/routes/preview.ts (1)
215-243:⚠️ Potential issue | 🟠 MajorPass the CSP nonce to preview handle pages too.
/preview/choose-handleand/preview/choose-handle-pickerstill renderrenderChooseHandlePage()withoutres.locals.cspNonce, so their inline handle-checking script will be blocked by the new nonce-only CSP.🔒 Proposed fix
renderChooseHandlePage( FAKE_HANDLE_DOMAIN, queryString(req, 'error'), fakeCsrfToken(), true, css, + res.locals.cspNonce as string, ), @@ renderChooseHandlePage( FAKE_HANDLE_DOMAIN, queryString(req, 'error'), fakeCsrfToken(), false, css, + res.locals.cspNonce as string, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/preview.ts` around lines 215 - 243, The preview routes for '/preview/choose-handle' and '/preview/choose-handle-picker' call renderChooseHandlePage without including the CSP nonce, so the inline handle-checking script will be blocked; update both handlers (the async functions that call getBranding and sendHtml) to read res.locals.cspNonce and pass it into renderChooseHandlePage (alongside FAKE_HANDLE_DOMAIN, queryString(req,'error'), fakeCsrfToken(), the boolean flag, and css) so the page gets the nonce for its inline script.packages/auth-service/src/routes/login-page.ts (1)
394-413:⚠️ Potential issue | 🟠 MajorMove the OTP
oninputhandler into the nonce-stamped script.With
'unsafe-inline'removed fromscript-src, CSP blocks the inlineoninput="..."attribute on the OTP field; the nonce on line 413 authorizes only<script nonce="...">blocks, not inline event handlers. Attach this listener inside the script instead.Proposed fix
<input type="text" id="code" name="code" required maxlength="${opts.otpLength}" pattern="${inputProps.pattern}" inputmode="${inputProps.inputmode}" autocomplete="one-time-code" placeholder="${inputProps.placeholder}" class="otp-input" autocapitalize="${inputProps.autocapitalize}" - oninput="this.value=this.value.replace(/[\\s-]/g,'')" style="letter-spacing: ${Math.max(2, Math.round(32 / opts.otpLength))}px"> @@ var recoveryLink = document.getElementById('recovery-link'); + var codeInput = document.getElementById('code'); + + codeInput.addEventListener('input', function() { + this.value = this.value.replace(/[\\s-]/g, ''); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 394 - 413, Remove the inline oninput attribute from the OTP input (id="code", class="otp-input") and instead attach the same sanitizing listener inside the existing nonce-stamped script (the <script nonce="${escapeHtml(opts.cspNonce)}"> block); locate where the page writes the script and add document.getElementById('code')?.addEventListener('input', ...) to replace spaces and hyphens (same regex used inline), preserving behavior like maxlength/pattern/inputmode from opts and without adding any inline handlers so the CSP nonce authorizes the logic.packages/auth-service/src/index.ts (1)
40-50:⚠️ Potential issue | 🔴 CriticalUse timing-safe comparison for admin password authentication.
Line 76 compares the
Authorizationheader against a secret-derived value using!==. Replace withtimingSafeEqual()(available from@certified-app/shared) to prevent timing attacks:Suggested fix
import { timingSafeEqual } from '@certified-app/shared' // ... if (!authHeader || !timingSafeEqual(authHeader, expected)) { res.status(401).json({ error: 'Unauthorized' }) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/index.ts` around lines 40 - 50, The admin auth comparison currently uses a plain !== check on authHeader vs expected; import timingSafeEqual from '@certified-app/shared' and replace that direct comparison with a timing-safe check by converting both sides to Buffers (e.g., Buffer.from(authHeader) and Buffer.from(expected)) and calling timingSafeEqual(bufferA, bufferB); ensure you still handle missing authHeader or expected early (respond 401) and update the conditional that currently uses authHeader/expected !== to use the timingSafeEqual result instead.
🧹 Nitpick comments (3)
e2e/step-definitions/security.steps.ts (2)
56-64: UseHeaders.getSetCookie()for multi-valuedSet-Cookie.Node's
Headers.get('set-cookie')joins multipleSet-Cookievalues with,— but because cookieExpiresattributes themselves contain commas, the combined string is ambiguous and easy to misparse. For theepds_csrf=substring check this still works today, but it's fragile: any future check that tries to inspect the cookie value would be broken by the join. Prefer the dedicatedgetSetCookie()accessor (Node 20+/undici), which returnsstring[].Suggested change
-Then('the response sets a CSRF cookie', function (this: EpdsWorld) { - const { headers } = getCapturedResponse(this) - const setCookie = headers.get('set-cookie') ?? '' - if (!/epds_csrf=/.test(setCookie)) { +Then('the response sets a CSRF cookie', function (this: EpdsWorld) { + const { headers } = getCapturedResponse(this) + const cookies = headers.getSetCookie() + if (!cookies.some((c) => /^epds_csrf=/.test(c))) { throw new Error( - `Expected Set-Cookie to include epds_csrf=..., got: ${setCookie || '(none)'}`, + `Expected Set-Cookie to include epds_csrf=..., got: ${cookies.join(' | ') || '(none)'}`, ) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/security.steps.ts` around lines 56 - 64, Replace the fragile use of headers.get('set-cookie') in the Then step testing CSRF cookie with the multi-value accessor headers.getSetCookie(); in the step function (the Then handler using getCapturedResponse(this)), call headers.getSetCookie() to get a string[] (or empty array if undefined), then assert that some element matches /epds_csrf=/ and throw the same error message if none match, preserving getCapturedResponse(this) and the step name.
139-155:/preview/login404 fallback to/healthsilently weakens the CSP scenario.If the preview route is disabled in the environment under test, the step silently probes
/healthinstead. Since the middleware applies CSP globally, the nonce/unsafe-inlineassertions still pass — but the scenario's stated intent ("the login page is loaded") is no longer validated, and a regression where the login route specifically loses CSP would go undetected. Consider either requiring previews in the test env (fail on 404) or probing the real login route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/security.steps.ts` around lines 139 - 155, The step When('the login page is loaded', async function (this: EpdsWorld) currently falls back from previewUrl (`${testEnv.authUrl}/preview/login`) to `/health` on a 404, which weakens the intended "login page is loaded" assertion; change the behavior in this step so that a 404 on previewUrl fails the test (throw or set a failing assertion) or instead probe the real login route (e.g., `${testEnv.authUrl}/login` or the actual auth-service login endpoint) and use that response for setCapturedResponse(this, ...), ensuring the test validates the actual login page CSP rather than a generic `/health` response.packages/shared/src/preview-ui.ts (1)
491-494: Consider defensive escaping (or validation) ofcspNoncein the attribute.
cspNonceis injected raw into a double-quoted HTML attribute. In practice auth-service generates it viacrypto.randomBytes(16).toString('base64url')so it's restricted to[A-Za-z0-9_-]and safe, but this helper is exported and could be called by future code. A one-line guard (orescapeHtml) keeps the contract defensive against an accidental caller that passes through unvalidated input.Optional hardening
export function previewClientIdScriptHtml(cspNonce?: string): string { - const nonceAttr = cspNonce ? ` nonce="${cspNonce}"` : '' + if (cspNonce && !/^[A-Za-z0-9_-]+$/.test(cspNonce)) { + throw new Error('cspNonce must be base64url-safe') + } + const nonceAttr = cspNonce ? ` nonce="${cspNonce}"` : '' return `<script${nonceAttr}>\n${PREVIEW_CLIENT_ID_SCRIPT_BODY}</script>` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/preview-ui.ts` around lines 491 - 494, The previewClientIdScriptHtml helper currently injects cspNonce raw into nonceAttr; make this defensive by validating or escaping cspNonce before interpolation in previewClientIdScriptHtml (e.g., in the function sanitize the input used to build nonceAttr: either run a strict whitelist check for /^[A-Za-z0-9_-]+$/ and only use it when it passes, or pass it through an HTML-attribute escape helper like escapeHtml and fall back to empty string on failure), so update previewClientIdScriptHtml to compute a safe nonce value and then build nonceAttr from that safe value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/__tests__/security-headers.test.ts`:
- Around line 32-51: The withServer helper nests listen and close callbacks too
deeply; refactor by extracting small helpers to start and stop the server and
use async/await with try/finally to ensure cleanup: create a startServer
function that returns the server and bound port (use app.listen and AddressInfo)
and a closeServer helper that closes the server, then in withServer await
startServer(), call fn(baseUrl) inside a try block and always await
closeServer() in finally so you remove nested .then/.catch callbacks around
server.close and satisfy SonarCloud nesting rules.
In `@packages/auth-service/src/index.ts`:
- Around line 73-76: The direct string comparison authHeader !== expected must
be replaced with the constant-time helper timingSafeEqual to avoid timing
attacks: convert both values to Buffers (ensure authHeader is a string or use an
empty string if missing), check lengths equal, then call
timingSafeEqual(authBuf, expectedBuf) and branch on its boolean result instead
of !==; update the conditional around authHeader/expected (the symbols
authHeader, expected) to use this helper so secret comparisons are done in
constant time.
In `@packages/auth-service/src/middleware/security-headers.ts`:
- Around line 37-40: The code can pass a string[] to db.getAuthFlowByRequestUri
because req.query.request_uri is typed string | string[] | undefined; update the
check that sets clientId so it only calls db.getAuthFlowByRequestUri when typeof
req.query.request_uri === 'string' (e.g., guard req.query.request_uri with a
typeof check) and avoid casting with as string; reference the
req.query.request_uri usage and db.getAuthFlowByRequestUri call and ensure
clientId is only assigned from db.getAuthFlowByRequestUri(req.query.request_uri)
when the guard passes.
---
Outside diff comments:
In `@packages/auth-service/src/index.ts`:
- Around line 40-50: The admin auth comparison currently uses a plain !== check
on authHeader vs expected; import timingSafeEqual from '@certified-app/shared'
and replace that direct comparison with a timing-safe check by converting both
sides to Buffers (e.g., Buffer.from(authHeader) and Buffer.from(expected)) and
calling timingSafeEqual(bufferA, bufferB); ensure you still handle missing
authHeader or expected early (respond 401) and update the conditional that
currently uses authHeader/expected !== to use the timingSafeEqual result
instead.
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 315-326: The error branch that returns the handle picker omits the
CSP nonce, causing a mismatch between the page CSP and inline scripts; update
the render call in the catch block so renderChooseHandlePage receives
res.locals.cspNonce (like other error paths do) — locate the catch that logs
with logger.error({ err, fullHandle }, 'Failed to check handle availability')
and pass res.locals.cspNonce into the renderChooseHandlePage invocation along
with handleDomain, the error message, res.locals.csrfToken, showRandomButton,
and customCss.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 394-413: Remove the inline oninput attribute from the OTP input
(id="code", class="otp-input") and instead attach the same sanitizing listener
inside the existing nonce-stamped script (the <script
nonce="${escapeHtml(opts.cspNonce)}"> block); locate where the page writes the
script and add document.getElementById('code')?.addEventListener('input', ...)
to replace spaces and hyphens (same regex used inline), preserving behavior like
maxlength/pattern/inputmode from opts and without adding any inline handlers so
the CSP nonce authorizes the logic.
In `@packages/auth-service/src/routes/preview.ts`:
- Around line 215-243: The preview routes for '/preview/choose-handle' and
'/preview/choose-handle-picker' call renderChooseHandlePage without including
the CSP nonce, so the inline handle-checking script will be blocked; update both
handlers (the async functions that call getBranding and sendHtml) to read
res.locals.cspNonce and pass it into renderChooseHandlePage (alongside
FAKE_HANDLE_DOMAIN, queryString(req,'error'), fakeCsrfToken(), the boolean flag,
and css) so the page gets the nonce for its inline script.
---
Nitpick comments:
In `@e2e/step-definitions/security.steps.ts`:
- Around line 56-64: Replace the fragile use of headers.get('set-cookie') in the
Then step testing CSRF cookie with the multi-value accessor
headers.getSetCookie(); in the step function (the Then handler using
getCapturedResponse(this)), call headers.getSetCookie() to get a string[] (or
empty array if undefined), then assert that some element matches /epds_csrf=/
and throw the same error message if none match, preserving
getCapturedResponse(this) and the step name.
- Around line 139-155: The step When('the login page is loaded', async function
(this: EpdsWorld) currently falls back from previewUrl
(`${testEnv.authUrl}/preview/login`) to `/health` on a 404, which weakens the
intended "login page is loaded" assertion; change the behavior in this step so
that a 404 on previewUrl fails the test (throw or set a failing assertion) or
instead probe the real login route (e.g., `${testEnv.authUrl}/login` or the
actual auth-service login endpoint) and use that response for
setCapturedResponse(this, ...), ensuring the test validates the actual login
page CSP rather than a generic `/health` response.
In `@packages/shared/src/preview-ui.ts`:
- Around line 491-494: The previewClientIdScriptHtml helper currently injects
cspNonce raw into nonceAttr; make this defensive by validating or escaping
cspNonce before interpolation in previewClientIdScriptHtml (e.g., in the
function sanitize the input used to build nonceAttr: either run a strict
whitelist check for /^[A-Za-z0-9_-]+$/ and only use it when it passes, or pass
it through an HTML-attribute escape helper like escapeHtml and fall back to
empty string on failure), so update previewClientIdScriptHtml to compute a safe
nonce value and then build nonceAttr from that safe value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdf4ebf3-e701-4ff9-9c0e-bbb1ea21c749
📒 Files selected for processing (11)
.changeset/csp-nonce-and-metrics-auth.mde2e/step-definitions/security.steps.tsfeatures/security.featurepackages/auth-service/src/__tests__/security-headers.test.tspackages/auth-service/src/index.tspackages/auth-service/src/middleware/security-headers.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/shared/src/index.tspackages/shared/src/preview-ui.ts
Replace the auth service's Content-Security-Policy script-src 'unsafe-inline' with a per-response nonce. The security-headers middleware now generates a fresh base64url nonce on every request, stamps it into script-src, and exposes it via res.locals.cspNonce so templates can emit <script nonce="..."> for inline scripts. All inline scripts ePDS ships (login page, choose-handle page, preview index) are threaded through to read and stamp the nonce. Also tighten /metrics on the auth service: if PDS_ADMIN_PASSWORD is unset, return 401 instead of serving metrics unauthenticated, so a missing env var can't silently open the endpoint. Extract the inline security-headers middleware into its own module with dedicated unit tests (7 tests) covering the nonce contract, baseline headers, and client-origin img-src. Enable 5 previously-pending security.feature scenarios: two CSRF checks (targeting the server-rendered recovery form, which uses ePDS's own CSRF middleware rather than better-auth's), the security-headers table, the CSP check, and the metrics 401. New step definitions live in e2e/step-definitions/security.steps.ts.
780190d to
2755af3
Compare
Coverage Report for CI Build 24752082121Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.5%) to 42.967%Details
Uncovered Changes
Coverage Regressions25 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
- Thread cspNonce through 3 missed renderChooseHandlePage call sites (the /_internal/check-handle catch arm in choose-handle.ts, plus both /preview/choose-handle routes). Without the nonce argument the template emits a bare <script> tag which the strict CSP blocks, silently killing handle-picker interactivity on those paths. - Use timingSafeEqual() for the /metrics Basic-auth header check so the secret comparison doesn't leak timing information. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the auth-service by moving inline script execution under a per-response CSP nonce, tightening access to /metrics, and enabling end-to-end security scenarios to validate these guarantees.
Changes:
- Introduces per-response CSP nonces (dropping
script-src 'unsafe-inline') and threads the nonce through auth-service HTML templates and preview pages. - Locks down the auth-service
/metricsendpoint to return 401 whenPDS_ADMIN_PASSWORDis unset, and uses a timing-safe comparison when it is set. - Enables security Cucumber scenarios and adds unit tests around the extracted security-headers middleware (including nonce behavior).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/preview-ui.ts | Adds optional CSP nonce stamping for the preview index inline script; refactors script into body + helper. |
| packages/shared/src/index.ts | Re-exports previewClientIdScriptHtml for shared consumers. |
| packages/auth-service/src/routes/preview.ts | Passes res.locals.cspNonce into preview renderers so inline scripts can be nonced. |
| packages/auth-service/src/routes/login-page.ts | Requires and stamps CSP nonce onto the login page inline <script>. |
| packages/auth-service/src/routes/choose-handle.ts | Threads optional nonce into choose-handle page and conditionally stamps <script nonce=...>. |
| packages/auth-service/src/lib/security-headers.ts | Extracts/updates security headers middleware to generate per-response nonce and build CSP accordingly. |
| packages/auth-service/src/index.ts | Deny-by-default /metrics auth; uses timingSafeEqual for header comparison. |
| packages/auth-service/src/tests/security-headers.test.ts | Adds unit tests for nonce generation/contract and CSP script-src behavior. |
| features/security.feature | Enables security scenarios (CSRF, headers table, CSP nonce, metrics 401). |
| e2e/step-definitions/security.steps.ts | Implements HTTP-based Cucumber step definitions for the enabled security scenarios. |
| .changeset/csp-nonce-and-metrics-auth.md | Documents operator-facing behavior changes for CSP nonce and /metrics auth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Emit `WWW-Authenticate: Basic realm="metrics"` on every 401 from /metrics so HTTP Basic tooling is told which scheme to use (per RFC 7235). - Escape the CSP nonce before embedding it in the preview index's inline <script nonce="..."> attribute. Current callers pass a base64url crypto.randomBytes(16) value which is already attribute- safe, but escapeHtml here is cheap defence-in-depth if that ever changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extract the /metrics auth-check branching into a pure
checkMetricsAuth() helper in lib/metrics-auth.ts. The route handler
in index.ts becomes a thin wrapper that maps the helper's result
onto the Express response.
Covers every branch:
- adminPassword unset or empty → 401 even with a valid-looking header
- adminPassword set, header missing/empty → 401
- wrong password, wrong username, wrong scheme → 401
- length-matched-but-different payload → 401 (regression guard for
timingSafeEqual short-circuit behaviour)
- matching Basic admin:<password> → ok
Every 401 path carries WWW-Authenticate: Basic realm="metrics".
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The existing docstring claimed pds-core "doesn't set a CSP on preview pages", which was misleading — pds-core's /preview/consent route does set a CSP, it just uses 'unsafe-inline' rather than a nonce. Reword so the no-nonce branch is explicitly described as for CSPs that allow inline scripts without a nonce. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/lib/security-headers.ts (1)
10-12:⚠️ Potential issue | 🟡 MinorUpdate the purity note to include
res.localsmutation.The middleware now intentionally writes
res.locals.cspNonce, so the file-level comment saying there is “no res/req mutation outside the setHeader contract” is stale.📝 Proposed doc fix
- * The middleware factory + helpers are pure: no module-level state, - * no Express types, no res/req mutation outside the well-defined - * setHeader contract. That keeps them straightforward to unit-test + * The middleware factory + helpers have no module-level state and avoid + * Express types. The middleware mutates only the well-defined response + * surface: headers plus `res.locals.cspNonce`. That keeps them straightforward to unit-testAlso applies to: 167-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/lib/security-headers.ts` around lines 10 - 12, Update the purity note to reflect that the middleware intentionally mutates res.locals by writing res.locals.cspNonce: replace the phrase "no res/req mutation outside the setHeader contract" with a short note that the middleware and its helpers are pure except they intentionally set res.locals.cspNonce for CSP nonce propagation; apply the same wording to the duplicate doc block that currently repeats the stale claim so both the top-of-file purity comment and the later helper comment acknowledge the res.locals mutation.
🧹 Nitpick comments (1)
features/security.feature (1)
48-52: Also assert that inline scripts carry the advertised nonce.This scenario verifies the CSP header has a nonce, but it would still pass if the login page’s inline
<script>missed the matchingnonceattribute and got blocked by the browser. Add a step that extracts the nonce fromscript-srcand checks the page’s inline scripts use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/security.feature` around lines 48 - 52, Add a new assertion step to the "Content-Security-Policy restricts inline scripts" Scenario that parses the Content-Security-Policy header (from the existing "Content-Security-Policy header is present" step), extracts the nonce token from the script-src directive, and then inspects the loaded login page DOM (after "When the login page is loaded") to verify that each inline <script> element includes a matching nonce attribute; reference the existing scenario text ("script-src directive carries a per-response nonce" and "When the login page is loaded") and implement a step that fails if any inline script lacks a nonce or its nonce value does not equal the one parsed from the header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/security.steps.ts`:
- Around line 139-198: The test currently only checks that a nonce-shaped token
exists but not that it changes per response; update the Then step handler for
'the script-src directive carries a per-response nonce' to fetch the same
login/health endpoint twice (reuse the previewUrl/health probe logic from the
When handler), extract the script-src from each response using
getScriptSrcDirective, parse the nonce value from each script-src, and assert
the two nonces are different to prove freshness; keep the existing regex check
but add the second fetch + comparison and throw a descriptive error if nonces
match.
- Around line 56-64: Replace the unreliable headers.get('set-cookie') call in
the Then step that asserts a CSRF cookie (the step defined as Then('the response
sets a CSRF cookie' in security.steps.ts) which currently reads
getCapturedResponse(this) and uses headers.get) with the Fetch API's
Headers.getSetCookie() method; call getCapturedResponse(this) to obtain headers,
use headers.getSetCookie() (or fall back to '' if undefined) when building the
setCookie string, and then run the existing regex check (/epds_csrf=/) and error
path unchanged so multiple cookies and Node >=20 runtimes are handled correctly.
In `@packages/auth-service/src/lib/metrics-auth.ts`:
- Around line 54-57: The code calls timingSafeEqual(authHeader, expected) with
raw header text which can throw if lengths differ in bytes for non-ASCII
strings; update the check in the auth flow so you first validate and normalize
the header before calling timingSafeEqual: ensure authHeader is a string, create
Buffers for both the received header and expected value (e.g.,
Buffer.from(authHeader, 'utf8') and Buffer.from(expected, 'utf8'), check that
their byte lengths match and if not return UNAUTHORIZED, then call
timingSafeEqual on those Buffers (still returning UNAUTHORIZED on any mismatch)
— locate the logic around authHeader, expected, timingSafeEqual and UNAUTHORIZED
to implement this guard.
---
Outside diff comments:
In `@packages/auth-service/src/lib/security-headers.ts`:
- Around line 10-12: Update the purity note to reflect that the middleware
intentionally mutates res.locals by writing res.locals.cspNonce: replace the
phrase "no res/req mutation outside the setHeader contract" with a short note
that the middleware and its helpers are pure except they intentionally set
res.locals.cspNonce for CSP nonce propagation; apply the same wording to the
duplicate doc block that currently repeats the stale claim so both the
top-of-file purity comment and the later helper comment acknowledge the
res.locals mutation.
---
Nitpick comments:
In `@features/security.feature`:
- Around line 48-52: Add a new assertion step to the "Content-Security-Policy
restricts inline scripts" Scenario that parses the Content-Security-Policy
header (from the existing "Content-Security-Policy header is present" step),
extracts the nonce token from the script-src directive, and then inspects the
loaded login page DOM (after "When the login page is loaded") to verify that
each inline <script> element includes a matching nonce attribute; reference the
existing scenario text ("script-src directive carries a per-response nonce" and
"When the login page is loaded") and implement a step that fails if any inline
script lacks a nonce or its nonce value does not equal the one parsed from the
header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8771003-b873-4413-8d19-9b3c3fe7d65b
📒 Files selected for processing (13)
.changeset/csp-nonce-and-metrics-auth.mde2e/step-definitions/security.steps.tsfeatures/security.featurepackages/auth-service/src/__tests__/metrics-auth.test.tspackages/auth-service/src/__tests__/security-headers.test.tspackages/auth-service/src/index.tspackages/auth-service/src/lib/metrics-auth.tspackages/auth-service/src/lib/security-headers.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/shared/src/index.tspackages/shared/src/preview-ui.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/csp-nonce-and-metrics-auth.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/index.ts
- packages/auth-service/src/routes/login-page.ts
- packages/auth-service/src/tests/security-headers.test.ts
- shared.timingSafeEqual: compare UTF-8 byte lengths, not JS code units. Previously a non-ASCII input with the same .length as the expected value would reach crypto.timingSafeEqual with different byte lengths and throw RangeError — turning a 401 into a 500. Guards every caller (metrics-auth, recovery, callback signatures). - metrics-auth: regression test for non-ASCII Authorization header with matching code-unit length. - security.steps.ts: Headers.getSetCookie() for reliable multi-cookie access; nonce step now fetches twice and asserts nonces differ, so a hardcoded constant would no longer pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </div> | ||
|
|
||
| <script> | ||
| <script${cspNonce ? ` nonce="${escapeHtml(cspNonce)}"` : ''}> |
There was a problem hiding this comment.
The <script> tag conditionally omits the nonce when cspNonce is falsy. Since auth-service now emits a nonce-based CSP (script-src 'nonce-...'), omitting the nonce will break inline script execution and can mask wiring bugs. Consider making cspNonce required for this template (like renderLoginPage) and always stamping nonce="..." here.
| <script${cspNonce ? ` nonce="${escapeHtml(cspNonce)}"` : ''}> | |
| <script nonce="${escapeHtml(cspNonce)}"> |
| * CSP nonce to stamp on the inline <script>. Required on services that | ||
| * emit a `script-src 'nonce-...'` CSP (auth-service); omit on services | ||
| * that don't set a CSP (pds-core). |
There was a problem hiding this comment.
The doc comment for cspNonce implies pds-core "doesn't set a CSP". pds-core does set CSP headers on some preview routes (e.g. /preview/consent), so this wording is misleading. Reword this to be page-/policy-specific (e.g. callers that don’t use a nonce-based CSP for this page, or note that pds-core’s /preview index doesn’t set CSP).
| * CSP nonce to stamp on the inline <script>. Required on services that | |
| * emit a `script-src 'nonce-...'` CSP (auth-service); omit on services | |
| * that don't set a CSP (pds-core). | |
| * CSP nonce to stamp on the inline <script>. Required when this preview | |
| * index page is served with a `script-src 'nonce-...'` CSP (for example, | |
| * auth-service); omit when this page is served without a nonce-based | |
| * `script-src` policy. |



Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security Improvements
Tests